Skip to content

fix(generator): escape leading caret in non-negative character class#280

Open
greymoth-jp wants to merge 1 commit into
DmitrySoshnikov:masterfrom
greymoth-jp:fix-leading-caret-char-class
Open

fix(generator): escape leading caret in non-negative character class#280
greymoth-jp wants to merge 1 commit into
DmitrySoshnikov:masterfrom
greymoth-jp:fix-leading-caret-char-class

Conversation

@greymoth-jp

Copy link
Copy Markdown

regexp-tree.optimize('/[a^]/') returns /[^a]/, which inverts the regex: [a^] matches a or ^, while [^a] matches everything except a.

The optimizer's charClassClassrangesMerge sorts character-class members by code point, and ^ (U+005E) sorts before a (U+0061), so the class is reordered to put ^ first. The generator's CharacterClass handler emits the members verbatim, so the reordered class is written as [^a], and a leading ^ in a class is parsed back as negation.

It is more visible when the reordered class is short enough for the optimizer to keep:

optimize('/[a^bcdef]/')   was  /[^a-f]/   (negated, wrong)
                          now  /[\^a-f]/

The same path is reachable through the public generate() API: building or transforming an AST so that a non-negative class starts with a literal ^ produces an inverted regex today.

The fix is in the generator: when a character class is not negative and its first member is an unescaped literal ^, escape it. [a^] then round-trips correctly, and optimize('/[a^]/') returns /[a^]/ again. Genuine negative classes and an already-escaped [\^a] are unaffected.

Tests added for the generator and for the char-class-classranges-merge transform. The full src suite passes.

A non-negative character class whose first element is a literal `^`
was generated as a bare `[^...]`, which parses back as a negated class.
This surfaces through the optimizer: charClassClassrangesMerge sorts class
members by code point, so `/[a^]/` becomes `/[^a]/` (`^` is U+005E, before
`a`), inverting the regex. Calling generate() on a hand-built or otherwise
reordered AST hits the same path.

Escape a leading literal `^` when the class is not negative.
@DmitrySoshnikov

Copy link
Copy Markdown
Owner

@greymoth-jp - appreciate the fix. What's your take whether we should be optimizing /[a^]/ to /[\a^]/ at all? Worth it keep the original expression - assuming the bug is fixed?

@greymoth-jp

Copy link
Copy Markdown
Author

Thanks for taking a look.

I'd keep the generator fix regardless of what the optimizer does. generate() is public, so any non-negative class whose first member is a literal ^ has to come out as [\^...] or it round-trips into a negative class. That can come from a hand-built AST or any transform that reorders the members, not just this optimizer path, so escaping it in CharacterClass covers all of them at the single place that serializes classes.

On your actual question: I agree the optimizer gains nothing by reordering /[a^]/. The merge sorts members by code point to coalesce adjacent chars into ranges, but here it merges nothing and the output is the same length (one char longer once ^ is escaped), so it's churn with no payoff. Having charClassClassrangesMerge leave a class alone when a reorder wouldn't actually merge anything would keep /[a^]/ intact like you're suggesting, and seems worth doing on its own.

My preference would be to land this generator fix as the correctness floor for generate(), and do the "skip no-op reorders" change as a separate follow-up so it can be reviewed on its own. Happy to fold it in here instead if you'd rather keep it to one PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants